refactor(hash-aggr): Support spilling for single mode aggregation - #23965
refactor(hash-aggr): Support spilling for single mode aggregation#239652010YOUY01 wants to merge 1 commit into
Conversation
github-actions
Bot
added
sqllogictest
| Field::new("b", DataType::Float64, false), | ||
| ])); | ||
|
|
||
| let group_keys = [2, 3, 4, 4].repeat(1_000); |
There was a problem hiding this comment.
The old implementation set a very tight memory budget, and the refactored implementation might have minor implementation difference, making this test fail.
Here it just try to scale up the test input size and memory limit, the test target is the same.
| async fn test_aggregate_with_spill_if_necessary() -> Result<()> { | ||
| // test with spill | ||
| run_test_with_spill_pool_if_necessary(2_000, true).await?; | ||
| run_test_with_spill_pool_if_necessary(20_000, true).await?; |
There was a problem hiding this comment.
Here is the same as the previous note on test change.
| matches!(root, DataFusionError::ResourcesExhausted(_)), | ||
| "Expected ResourcesExhausted, got: {root}", | ||
| ); | ||
| let msg = root.to_string(); |
There was a problem hiding this comment.
Above check on error type is enough I think, here it also assert the exact error message
| /// See comments at `poll_next()` for details. | ||
| /// | ||
| /// Returns the next operator state with control flow decision. | ||
| fn handle_producing_output( |
There was a problem hiding this comment.
This function has no functional changes, the diffs are
- use internal_err instead of assert
- simplify some control flow code with
break_with_err
| SET datafusion.execution.target_partitions = 1 | ||
|
|
||
| statement ok | ||
| SET datafusion.execution.batch_size = 128 |
There was a problem hiding this comment.
similar to the above notes on the test change #23965 (comment)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23965 +/- ##
==========================================
+ Coverage 80.65% 80.75% +0.09%
==========================================
Files 1093 1096 +3
Lines 371619 373556 +1937
Branches 371619 373556 +1937
==========================================
+ Hits 299730 301659 +1929
+ Misses 53993 53907 -86
- Partials 17896 17990 +94 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Which issue does this PR close?
Part of #22710
Rationale for this change
This PR implements larger-than-memory execution for single mode aggregation.
See top comments at
single_stream.rsfor the high-level idea. The key implementations are in the same file.What changes are included in this PR?
Are these changes tested?
Existing tests.
Are there any user-facing changes?
No